Skip to content

Conversation

@depatl
Copy link
Collaborator

@depatl depatl commented Apr 29, 2025

This change is Reviewable

Copy link
Owner

@juledwar juledwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

We also need to update the Github workflows to test the MacOS path, it needs a
runs-on: macos-latest in there, but the test of the steps should be identical.

Copy link
Owner

@juledwar juledwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so there's a few things to fix:

  1. The imports need formatting:
    cmd [3] | ruff check --select I --show-fixes dbtesttools
    dbtesttools/tests/test_pgfixture.py:1:1: I001 [*] Import block is un-sorted or un-formatted
  2. The GH workflows are failing because Podman is not installed. This might help: https://github.com/marketplace/actions/install-podman
  3. We need to add the macos runner to the test matrix in GH workflows.

The good news is that the tests all pass for me locally \o/

@juledwar
Copy link
Owner

juledwar commented May 1, 2025

I have no idea why your branch is failing the hatch build long_description check - I can recreate it locally but only on your branch and not on master!

But there's a bunch of other error output from the tests, which are still passing, so something is not quite right.

You also still need to make sure podman is installed and running.

@juledwar
Copy link
Owner

juledwar commented May 1, 2025

I suspect most of the errors are because podman is not up and running in the GH container that's running the tests.

add podman support

cleanup

lint

add socket verification

add scenarios and prevent early termination errors

add Podman related instructions

ruff lint

add macos target to github actions

fixing typo

add docker and podman runtimes to gh actions workflow

switch to docker-ce on ubuntu

split test on podam and docker into 2 separate jobs

split podman and docker tests on macos

Podman and docker can't coexist on macos. thus running it separately

fixing path

switch to colima to run docker cli

handle podman startup

only test podman on MacOs

removing mess

remove macos and separate podman and docker on ubuntu

adjust path to tests

bogus commit to rerun the workflow

fix freaking twine error

typo

indentation

indentation
gnupg \
lsb-release
sudo mkdir -p /etc/apt/keyrings
curl -fsSL https://download.docker.com/linux/ubuntu/gpg | sudo gpg --dearmor -o /etc/apt/keyrings/docker.gpg
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on curl here is very bad, it means the tests won't pass if that site is down or something similar is broken.

# Override the scenarios to only run Podman
TestPostgresContainer.scenarios = [
("podman", {"podman": True}),
]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you're doing this, and the previous change. The scenarios declaration will run both of these automatically. This change means that the last file to to get imported overrides the scenarios as well, so for example on my machine the only scenario that gets run now is podman.

When running hatch run ci locally it should do exactly the same tests as Workflows would run, which is why the initial workflow file was as simple as it was, with the addition of the Python test matrix (which should arguably be declared in the Hatch matrix, but that's on my list of things to fix later).

In other words, we should not depend on GH Actions to run all of our tests to completion.

Let's go back to basics with this change and simply run up a worker with podman installed. See here https://github.com/marketplace/actions/install-podman for one in the marketplace.

I don't think you need all that docker-ce stuff as well, was there a reason to do it?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah apparently that podman action is deprecated, crap. Back to manual installation.

@juledwar
Copy link
Owner

juledwar commented May 6, 2025

Also it was not indentation that cause the RST rendering break, it was the lack of a blank line. Here's a patch for you:

diff --git README.rst README.rst
index 7d3eae1..ea05e55 100644
--- README.rst
+++ README.rst
@@ -94,12 +94,13 @@ PG containers. You will need to do up to two extra things:
 
 If you want to use Podman instead of Docker engine you will need to follow
 these steps:
-1. Enable and start podman on your host
-```bash
-systemctl --user enable podman
-systemctl --user start podman
-```
-2. `export DBTESTTOOLS_USE_PODMAN=1` variable. 
+
+    1. Enable and start podman on your host
+    ```bash
+    systemctl --user enable podman
+    systemctl --user start podman
+    ```
+    2. `export DBTESTTOOLS_USE_PODMAN=1` variable.
 
 This code has been in use daily on a large project at Cisco for a few years
 now, and is very stable.

Copy link
Owner

@juledwar juledwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the changes I suggested and pushed a new branch up:
https://github.com/juledwar/db-testtools/pull/17/files
I also simplified the Actions config. You can copy the changes to your own branch or just merge mine.

@juledwar
Copy link
Owner

juledwar commented May 6, 2025

Closing in favour of #17

@juledwar juledwar closed this May 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants